Add httpMaxRetryCount && Simplify http fallback flow by gokhanbarisaker · Pull Request #80 · ably/ably-java

gokhanbarisaker · 2016-01-29T11:46:46Z

Resolves: #54, #14

gokhanbarisaker · 2016-01-29T11:48:30Z

Test cases for that will require utilising HostFailedException. It will be available once we resolve #74

paddybyers · 2016-01-30T11:17:31Z

If this is private we don't necessarily need a javadoc comment, but please either add a comment or remove the TODO. It's not worth leaving like this.

paddybyers · 2016-01-30T11:26:30Z

I like this, but please see the comments above.

mattheworiordan · 2016-02-01T11:26:51Z

Am I missing something here, as far as I can tell we have removed the fallback host functionality fro Realtime connections in this commit? The Random class is not even used anymore, and the connection implementation still references fallback hosts yet does not use them, see https://github.com/gokhanbarisaker/ably-java/blob/8b59f05590e123462f2603ffccdd9a696f4e0434/lib/src/main/java/io/ably/lib/transport/ConnectionManager.java#L574-L578

paddybyers · 2016-02-04T22:50:01Z

we don't necessarily default to the last successful fallback host

The previous functionality wasn't reverting to the last successful host for an HTTP request. It was saying that if this is a realtime instance, and it is currently connected, then that host is the preferred host.

mattheworiordan · 2016-02-08T08:48:30Z

It was saying that if this is a realtime instance, and it is currently connected, then that host is the preferred host.

The activeHost is set when the Http class is instanced for the REST lib. As far as I can tell, every REST request then uses activeHost, and if that request fails just once, activeHost is set to a random fallback host for all remaining requests. Am I missing something, what has the realtime connection got to do with that?

gokhanbarisaker · 2016-02-08T12:35:51Z

@mattheworiordan

The activeHost is set when the Http class is instanced for the REST lib. As far as I can tell, every REST request then uses activeHost, and if that request fails just once, activeHost is set to a random fallback host for all remaining requests. Am I missing something, what has the realtime connection got to do with that?

I have already updated that behavior with latest commit. Now we don't have persistent fallback for host variable.

mattheworiordan · 2016-02-09T13:14:25Z

As far as I can tell from this PR, the following is not being done correctly:

Preferred REST host when Realtime is connected no longer works as you removed it
All REST requests from the Realtime library (such as Channel history) now always use the incorrect Realtime endpoint, see https://github.com/ably/ably-java/pull/80/files#diff-b8b1ac0a34230df724c2576f32005521R54
You are only catching HostFailedException for fallback attempts, but this is not broad enough for a rest request retry, see http://docs.ably.io/client-lib-development-guide/features/#RSC15d. Same applies for realtime
A style concern admittedly, but I really dislike evaluating variables that include a postfix or prefix operation, see https://github.com/ably/ably-java/pull/80/files#diff-8a8a56ece4e68a2d0fbeb6639c2e5469R228. Could we not decrement the retry variable explicitly when we make the attempt to simplify readability of this code?
My comment about inconsistent use of upper case as well as missing underscores is still not addressed
Setting httpMaxRetryCount in all tests seems unnecessary, see https://github.com/ably/ably-java/pull/80/files#diff-d86628d178e27c35757c30abecfa69f3R75. I don't think you responded to that one?
Not sure how FALLBACKS could ever be empty given it's static and final, see https://github.com/ably/ably-java/pull/80/files#diff-633aa103b8a41039ef0c67f40e6a610aR21 and https://github.com/ably/ably-java/pull/80/files#diff-060685a947a7032665de7ac9d33bb6ffR486

BTW. Test coverage is looking good :)

gokhanbarisaker · 2016-02-09T18:53:12Z

Preferred REST host when Realtime is connected no longer works as you removed it

I am setting it explicitly on AblyRealtime constructor. https://github.com/ably/ably-java/pull/80/files#diff-b8b1ac0a34230df724c2576f32005521R54

All REST requests from the Realtime library (such as Channel history) now always use the incorrect Realtime endpoint, see https://github.com/ably/ably-java/pull/80/files#diff-b8b1ac0a34230df724c2576f32005521R54

Done, gokhanbarisaker@9ede727

You are only catching HostFailedException for fallback attempts, but this is not broad enough for a rest request retry, see http://docs.ably.io/client-lib-development-guide/features/#RSC15d. Same applies for realtime

Done, gokhanbarisaker@b21c73c

A style concern admittedly, but I really dislike evaluating variables that include a postfix or prefix operation, see https://github.com/ably/ably-java/pull/80/files#diff-8a8a56ece4e68a2d0fbeb6639c2e5469R228. Could we not decrement the retry variable explicitly when we make the attempt to simplify readability of this code?

Done, gokhanbarisaker@55746eb

My comment about inconsistent use of upper case as well as missing underscores is still not addressed

Done, gokhanbarisaker@0fcb999

Setting httpMaxRetryCount in all tests seems unnecessary, see https://github.com/ably/ably-java/pull/80/files#diff-d86628d178e27c35757c30abecfa69f3R75. I don't think you responded to that one?

Done, gokhanbarisaker@9e07f45

Not sure how FALLBACKS could ever be empty given it's static and final, see https://github.com/ably/ably-java/pull/80/files#diff-633aa103b8a41039ef0c67f40e6a610aR21 and https://github.com/ably/ably-java/pull/80/files#diff-060685a947a7032665de7ac9d33bb6ffR486

Done, gokhanbarisaker@5e8a523

paddybyers · 2016-02-09T18:55:12Z

 		super(options);
 		channels = new Channels();
 		connection = new Connection(this);
+		http.setHost(options.realtimeHost);


You're missing the point. The host that you happen to be connected to will change from time to time, because the fallback hosts behaviour applies to realtime connections, not just rest requests. So it won't work to freeze the realtime host here for the lifetime of the library.

I solved that issue here https://github.com/ably/ably-java/pull/80/files#diff-060685a947a7032665de7ac9d33bb6ffR588

I see two problems with this approach:

You are setting the REST Host to the RealtimeHost in all situations. This is wrong, we only use the fallback host for REST requests when the realtime host is using that fallback host.

If a realtime request falls back to a fallback host, but then later resumes using the default realtime host, all future REST requests will continue to go to the incorrect fallback host, see https://github.com/ably/ably-java/pull/80/files#diff-060685a947a7032665de7ac9d33bb6ffR588

paddybyers · 2016-02-09T18:55:39Z

I am setting it explicitly on AblyRealtime constructor

See #80 (comment)

mattheworiordan · 2016-02-09T19:50:36Z

 			opts.port = testVars.port;
 			opts.tlsPort = testVars.tlsPort;
 			opts.tls = testVars.tls;
+			opts.httpMaxRetryCount = testVars.httpMaxRetryCount;


All of these httpMaxRetryCount configurations are still here and not related to the test in hand at all? Why can't they simply use the default?

mattheworiordan · 2016-02-09T19:51:17Z

@gokhanbarisaker I've added a few more comments...

gokhanbarisaker · 2016-02-11T13:57:05Z

No test for a 500 response? Given the changes you have made here, it would be nice to know that this works as intended and that we don't have a reg…

Done, gokhanbarisaker@d196d3f

Should we not have an else here to ensure that the host is reset to the default REST endpoint when fallback is false?

Absolutely. Done, gokhanbarisaker@af2b5fb

This should be changed still, it should be HTTP_MAX_RETRY_COUNT i.e. you have to use underscores consistently for word breaks

Done, gokhanbarisaker@25cf247

All of these httpMaxRetryCount configurations are still here and not related to the test in hand at all? Why can't they simply use the default?

Done, gokhanbarisaker@1c6e9c2

Also, I have updated the fallback handling behavior.

I think the logic is slightly wrong here, it should be while (retryCountRemaining >= 0); Example: Assume max retries is 2 First run against primar…

Done, gokhanbarisaker@fbba8db

mattheworiordan · 2016-02-12T09:39:31Z

 			ClientOptions opts = new ClientOptions(testVars.keys[0].keyStr);
 			opts.restHost = "some.other.host";
 			ably = new AblyRealtime(opts);
-			assertEquals("Unexpected host mismatch", Defaults.getHost(opts), opts.restHost);


Why is this removed? I am assuming you want to configure realtimeHost and ensure it is used, but perhaps check that restHost remains unchanged, then change both and ensure both are used?

AblyRealtime is using Http#setHost https://github.com/ably/ably-java/pull/80/files#diff-8a8a56ece4e68a2d0fbeb6639c2e5469R136

So this worries me on two counts. Firstly, if you ave a test without an assertion, this is not a test. Secondly, this is a realtime test which is supposed to test that when the REST host is set, all rest requests go to that host. Equally, if a realtimeHost is set, then all realtime requests go to that host. We need that test as I am not confident that is working without suitable test coverage.

paddybyers reviewed Jan 30, 2016
View reviewed changes

gokhanbarisaker force-pushed the RSC15 branch 2 times, most recently from 7ab0be6 to 8b59f05 Compare January 30, 2016 11:55

mattheworiordan reviewed Feb 1, 2016
View reviewed changes

paddybyers reviewed Feb 9, 2016
View reviewed changes

mattheworiordan reviewed Feb 9, 2016
View reviewed changes

mattheworiordan reviewed Feb 12, 2016
View reviewed changes

Conversation

gokhanbarisaker commented Jan 29, 2016

Uh oh!

gokhanbarisaker commented Jan 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paddybyers commented Jan 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paddybyers commented Feb 4, 2016

Uh oh!

mattheworiordan commented Feb 8, 2016

Uh oh!

gokhanbarisaker commented Feb 8, 2016

Uh oh!

mattheworiordan commented Feb 9, 2016

Uh oh!

gokhanbarisaker commented Feb 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paddybyers commented Feb 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattheworiordan commented Feb 9, 2016

Uh oh!

gokhanbarisaker commented Feb 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants